Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR elastic#137964

Type: Clean (correct implementation)

Original PR Title: Fix for GET /_migration/deprecations doesn't report node deprecations if low watermark exceeded and GET /_migration/deprecations doesn't report node-level failures properly
Original PR Description: Closes elastic#137010
Closes elastic#137004
Original PR URL: elastic#137964


PR Type

Bug fix


Description

  • Fix node deprecation reporting when low watermark exceeded

  • Improve node-level failure logging with proper exception context

  • Refactor deprecation check logic to include watermark issues

  • Add comprehensive test for disk watermark deprecation warnings


Diagram Walkthrough

flowchart LR
  A["Node Deprecation Checker"] -->|logs failures| B["Enhanced Logging"]
  C["Transport Node Deprecation Check"] -->|refactored| D["Improved Issue Collection"]
  D -->|includes| E["Disk Watermark Issues"]
  F["Test Suite"] -->|validates| E
Loading

File Walkthrough

Relevant files
Bug fix
NodeDeprecationChecker.java
Enhanced failure logging with exception context                   

x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecker.java

  • Changed debug logging to warn level with exception context
  • Uses logger.atWarn().withThrowable() for better error reporting
  • Improves visibility of node-level deprecation check failures
+1/-1     
TransportNodeDeprecationCheckAction.java
Refactor deprecation issue collection logic                           

x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportNodeDeprecationCheckAction.java

  • Removed unused NodeRequest parameter from nodeOperation method
  • Replaced stream-based filtering with explicit loop for null handling
  • Ensures disk watermark issues are properly included in results
  • Removed unnecessary Objects import
+14/-7   
Tests
TransportNodeDeprecationCheckActionTests.java
Add watermark deprecation test and update signatures         

x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/TransportNodeDeprecationCheckActionTests.java

  • Updated test calls to match refactored nodeOperation signature
  • Added new test testDiskLowWatermarkIsIncludedInDeprecationWarnings
  • Validates that disk watermark issues appear alongside other
    deprecations
  • Added custom DeprecationIssueMatcher for flexible assertion matching
  • Added necessary imports for test utilities and matchers
+102/-4 
Documentation
137964.yaml
Add changelog entry for bug fix                                                   

docs/changelog/137964.yaml

+9/-0     

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Logging Context: The PR adds warn-level logs for node failures, but does not include explicit user identity
or structured fields, making it unclear if audit requirements are met for these critical
actions.

Referred Code
    logger.atWarn().withThrowable(failure).log("node {} failed to run deprecation checks", failure.nodeId());
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input Assumptions: The change replaces stream filtering with a loop and processes deprecation checks without
visible validation of external inputs (e.g., settings or cluster info), which may rely on
validations outside the shown diff.

Referred Code
List<DeprecationIssue> issues = new ArrayList<>();
for (NodeDeprecationChecks.NodeDeprecationCheck<
    Settings,
    PluginsAndModules,
    ClusterState,
    XPackLicenseState,
    DeprecationIssue> c : nodeSettingsChecks) {
    DeprecationIssue deprecationIssue = c.apply(filteredNodeSettings, pluginsService.info(), filteredClusterState, licenseState);
    if (deprecationIssue != null) {
        issues.add(deprecationIssue);
    }
}
DeprecationIssue watermarkIssue = checkDiskLowWatermark(

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use instanceof pattern matching

Simplify the matches method by using instanceof pattern matching to combine the
type check and variable binding into a single, more concise step.

x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/TransportNodeDeprecationCheckActionTests.java [289-296]

 @Override
 public boolean matches(Object actual) {
-    if (actual instanceof DeprecationIssue == false) {
-        return false;
+    if (actual instanceof DeprecationIssue actualDeprecationIssue) {
+        return level.equals(actualDeprecationIssue.getLevel()) && messageMatcher.matches(actualDeprecationIssue.getMessage());
     }
-    DeprecationIssue actualDeprecationIssue = (DeprecationIssue) actual;
-    return level.equals(actualDeprecationIssue.getLevel()) && messageMatcher.matches(actualDeprecationIssue.getMessage());
+    return false;
 }
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly proposes using instanceof pattern matching for better readability, but it's a minor style improvement in a test helper class with low impact.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants